Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for sql auditing to Azure Monitor #10324

Merged
merged 4 commits into from
Feb 26, 2021

Conversation

yupwei68
Copy link
Contributor

@yupwei68 yupwei68 commented Jan 27, 2021

Fix: #7695
#10100
#6849

=== RUN TestAccMsSqlDatabaseExtendedAuditingPolicy_basic
=== PAUSE TestAccMsSqlDatabaseExtendedAuditingPolicy_basic
=== CONT TestAccMsSqlDatabaseExtendedAuditingPolicy_basic
--- PASS: TestAccMsSqlDatabaseExtendedAuditingPolicy_basic (358.57s)
=== RUN TestAccMsSqlDatabaseExtendedAuditingPolicy_requiresImport
=== PAUSE TestAccMsSqlDatabaseExtendedAuditingPolicy_requiresImport
=== CONT TestAccMsSqlDatabaseExtendedAuditingPolicy_requiresImport
--- PASS: TestAccMsSqlDatabaseExtendedAuditingPolicy_requiresImport (391.47s)
=== RUN TestAccMsSqlDatabaseExtendedAuditingPolicy_complete
=== PAUSE TestAccMsSqlDatabaseExtendedAuditingPolicy_complete
=== CONT TestAccMsSqlDatabaseExtendedAuditingPolicy_complete
--- PASS: TestAccMsSqlDatabaseExtendedAuditingPolicy_complete (422.68s)
=== RUN TestAccMsSqlDatabaseExtendedAuditingPolicy_update
=== PAUSE TestAccMsSqlDatabaseExtendedAuditingPolicy_update
=== CONT TestAccMsSqlDatabaseExtendedAuditingPolicy_update
--- PASS: TestAccMsSqlDatabaseExtendedAuditingPolicy_update (547.36s)
=== RUN TestAccMsSqlDatabaseExtendedAuditingPolicy_storageAccBehindFireWall
=== PAUSE TestAccMsSqlDatabaseExtendedAuditingPolicy_storageAccBehindFireWall
=== CONT TestAccMsSqlDatabaseExtendedAuditingPolicy_storageAccBehindFireWall
--- PASS: TestAccMsSqlDatabaseExtendedAuditingPolicy_storageAccBehindFireWall (354.62s)

=== RUN TestAccMsSqlDatabase_withBlobAuditingPolices
=== PAUSE TestAccMsSqlDatabase_withBlobAuditingPolices
=== CONT TestAccMsSqlDatabase_withBlobAuditingPolices
--- PASS: TestAccMsSqlDatabase_withBlobAuditingPolices (564.62s)
=== RUN TestAccMsSqlDatabase_withBlobAuditingPolicesForMonitor
=== PAUSE TestAccMsSqlDatabase_withBlobAuditingPolicesForMonitor
=== CONT TestAccMsSqlDatabase_withBlobAuditingPolicesForMonitor
--- PASS: TestAccMsSqlDatabase_withBlobAuditingPolicesForMonitor (803.68s)

All regression tests have passed.

Copy link
Collaborator

@aristosvo aristosvo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I really like this PR! I started basically the same yesterday, but yours is far more complete!!

I have a few small change suggestions for the examples, code looks good to me but I leave that to one of the HC folks to decide 😅

Comment on lines +1170 to +1176
metric {
category = "AllMetrics"

retention_policy {
enabled = false
}
}
Copy link
Collaborator

@aristosvo aristosvo Jan 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this metric should be enabled/disabled, right? Maybe only for clarity reasons, because I don't know what the default behavior is 😅

Suggested change
metric {
category = "AllMetrics"
retention_policy {
enabled = false
}
}
metric {
category = "AllMetrics"
enabled = false
retention_policy {
enabled = false
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comments. I copies the monitor_diagnotics_setting example from its acctest . And it should be by default true. But I'll wait to see hashicorp's review comments. Because here we should try to avoid ignoring changes for log and metrics.

Comment on lines +1127 to +1133
metric {
category = "AllMetrics"

retention_policy {
enabled = false
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
metric {
category = "AllMetrics"
retention_policy {
enabled = false
}
}
metric {
category = "AllMetrics"
enabled = false
retention_policy {
enabled = false
}
}


metric {
category = "AllMetrics"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs an enabled=false

Suggested change
enabled = false


metric {
category = "AllMetrics"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

Suggested change
enabled = false

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @yupwei68 - overall this looks goof but i wonder if we should change the property name to something that indicates its log analytics related? see my inline comment

@yupwei68
Copy link
Contributor Author

Hi @katbyte Thanks for your comments. Changes have been pushed. Please continue reviewing.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yupwei68 - this LGTM 👍

@katbyte katbyte merged commit 2598a62 into hashicorp:master Feb 26, 2021
@katbyte katbyte added this to the v2.50.0 milestone Feb 26, 2021
@katbyte katbyte added enhancement service/mssql Microsoft SQL Server labels Feb 26, 2021
katbyte added a commit that referenced this pull request Feb 26, 2021
@ghost
Copy link

ghost commented Mar 5, 2021

This has been released in version 2.50.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.50.0"
}
# ... other configuration ...

@varnav
Copy link
Contributor

varnav commented Mar 5, 2021

This example shows how to enable auditing for database. But is it possible to enable it on server level? It is possible to do via Web GUI, "auditing" tab has options: Storage, Log Analytics and Event Hub

https://docs.microsoft.com/en-us/azure/azure-sql/database/auditing-overview

@aristosvo
Copy link
Collaborator

aristosvo commented Mar 6, 2021

@varnav I believe the solution is to add a data resource for the master database in your server and adding auditing there:

data "azurerm_mssql_database" "master" {
  name      = "master"
  server_id = azurerm_mssql_server.default.id
}

resource "azurerm_mssql_server_extended_auditing_policy" "example" {
  server_id       = azurerm_mssql_server.default.id
  monitor_enabled = true
}

data "azurerm_monitor_diagnostic_categories" "default" {
  resource_id = data.azurerm_mssql_database.master.id
}

// all diagnostics enabled
resource "azurerm_monitor_diagnostic_setting" "default" {
  name                       = "diagnostics"
  target_resource_id         = data.azurerm_mssql_database.master.id
  log_analytics_workspace_id = azurerm_log_analytics_workspace.default.id

  dynamic "log" {
    iterator = log_category
    for_each = data.azurerm_monitor_diagnostic_categories.default.logs

    content {
      category = log_category.value
      enabled  = true

      retention_policy {
        enabled = true
        days    = 0
      }
    }
  }

  dynamic "metric" {
    iterator = metric_category
    for_each = data.azurerm_monitor_diagnostic_categories.default.metrics

    content {
      category = metric_category.value
      enabled  = true

      retention_policy {
        enabled = true
        days    = 0
      }
    }
  }
}

I'll try it and look if I can add a test and example for it

@ghost
Copy link

ghost commented Mar 28, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Mar 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

azurerm_sql_server: extended_auditing_policy - send logs to Log Analytics and Event hub
4 participants